-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept literals in attribute names #396
Accept literals in attribute names #396
Conversation
Tried this, I seem to get:
|
Could you send more context like the macro where this element is created. This looks like and issue with missing |
|
I'm unable to reproduce the issue when copying the elements you sent. On the code that includes this PR it compiles for me and seemingly give the correct html. This is the html string your code produces: |
Nvm. Seems to be working now. I think I may have just forgotten to clear the build cache. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RedPhoenixQ, looks great so far!
Can you please add a changelog entry as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RedPhoenixQ!
Let's merge what we have now. We can address my other comment in a follow-up PR.
html! { this b"byte_string"="false" 123="123" 2.5 true 'a'="a" b'b'="b" of-course {} }; | ||
assert_eq!( | ||
result.into_string(), | ||
r#"<this byte_string="false" 123="123" 2.5 true a="a" b="b" of-course></this>"# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think string literals, and maybe integers, are enough.
Floating point numbers overlap with class shorthand which also uses .
. There's no use case for a single character. And no other part of Maud accepts byte strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lambda-fairy, addressing this issue, to keep ast::name_to_string()
as an infallible api (always returning String
without errors) I handled all the cases since I didn't see any issue with also handling ByteStr
and others so they produce the expected internal string. Current implementation should always produce valid html.
The class shorthand shouldn't be broken since classes starting with numbers are disallowed in CSS so all classes before should still be possible. Also the only time a class starting with a number wouldn't be interpreted as a class would be 123.123
(space followed by numbers with dot and no spaces), but 123 .123
, anything.123
and .classname123.123
would still parse as a class shorthand for the class 123
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the interpretation change just by adding a space is the confusing part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#398 fixes this issue by running the existing literal()
function in the parser when encountering a literal, reusing existing error behaviour. I've added Int
as an accepted literal, but can revoke that change if you decide that Int
shouldn't be allowed.
I found a bug. Literals are now parsed and the |
The parser now accepts
TokenTree::Literal
as names for attributes.To prevent the
name_to_string
function from showing and escaping the surrounding"
of a string literal the string representation is trimmed first. This should not affect how other literals work which means that both"name"="true"
and123="true"
would be valid.This also addresses the PR request in #194